Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portable SIMD subtree update #135701

Open
wants to merge 104 commits into
base: master
Choose a base branch
from

Conversation

calebzulawski
Copy link
Member

workingjubilee and others added 30 commits July 7, 2023 04:07
Signed-off-by: cui fliter <[email protected]>
add without_provenance to pointer types
Add loongarch64 vendor conversions
…r=Amanieu

rename ptr::from_exposed_addr -> ptr::with_exposed_provenance

As discussed on [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/To.20expose.20or.20not.20to.20expose/near/427757066).

The old name, `from_exposed_addr`, makes little sense as it's not the address that is exposed, it's the provenance. (`ptr.expose_addr()` stays unchanged as we haven't found a better option yet. The intended interpretation is "expose the provenance and return the address".)

The new name nicely matches `ptr::without_provenance`.
Avoid migration pitfall from packed_simd as described in rust-lang#402.
document PartialOrd difference to simd_min and simd_max
@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2025

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2025
@calebzulawski
Copy link
Member Author

r? @workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@calebzulawski
Copy link
Member Author

@RalfJung (not sure who to ping for subtrees 🙃) does it make sense for some commits from rust-lang/rust to end up in this PR a second time? For example 989660c is in here a second time as b039861, I believe due to a merge conflict from the subtree-push to rust-lang/portable-simd.

@rust-log-analyzer

This comment has been minimized.

@programmerjake
Copy link
Member

programmerjake commented Jan 18, 2025

@RalfJung (not sure who to ping for subtrees 🙃) does it make sense for some commits from rust-lang/rust to end up in this PR a second time? For example 989660c is in here a second time as b039861, I believe due to a merge conflict from the subtree-push to rust-lang/portable-simd.

that shouldn't happen from a merge conflict since all that does is change the tree you end up with in the merge commit. you'd get duplicate commits if you did any rebasing (which it sounds like is what happened before). duplicate commits may also be fine and expected (I'm not 100% sure since I last researched subtrees a while ago...)

@calebzulawski
Copy link
Member Author

Ah, so rebasing subtree pushes in the past (not the most recent one) was a mistake because it rewrote that commit. That makes sense. I wonder if there's a fix now

@programmerjake
Copy link
Member

Ah, so rebasing subtree pushes in the past (not the most recent one) was a mistake because it rewrote that commit. That makes sense. I wonder if there's a fix now

I think the fix is probably to say oh well, we messed up before, we'll just end up with some duplicates but if the final repo contents is as expected then it's mostly ok since that's what matters most rather than having perfect git history

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2025

The Miri subtree was changed

cc @rust-lang/miri

@calebzulawski
Copy link
Member Author

Ready to merge this as long as @rust-lang/miri is ok with removing those test cases. We now only provide bitmasks to u64 and expect users to use lane shifting + bitmask to get beyond the first 64 lanes.

@RalfJung
Copy link
Member

RalfJung commented Jan 19, 2025

@RalfJung (not sure who to ping for subtrees 🙃)

portable-simd doesn't use josh, does it? I know next to nothing about git subtree, so I am definitely the wrong person to ping here. :) But yes, rebase-on-merge is generally a bad idea when using any of these tools.

The git subtree you are using is hosted on https://raw.githubusercontent.com/bjorn3 so @bjorn3 seems like a good starting point.

@RalfJung
Copy link
Member

RalfJung commented Jan 19, 2025

But why would there have been rebasing? You didn't recently change the subtree workflow, did you? Are you sure this will not keep causing issues for future syncs? Better test what happens when you merge this PR into the main repo and then sync that back, and vice versa. This subtree sync looks rather strange so something odd might be going on. The duplicated commits go all the way back to 2023! I assume there have been other syncs since 2023. So why were they not affected?

Clippy used git subtree for a long time, maybe they have some idea what is happening here -- Cc @rust-lang/clippy . I would be wary of merging this until this is resolved.

I think the fix is probably to say oh well, we messed up before, we'll just end up with some duplicates but if the final repo contents is as expected then it's mostly ok since that's what matters most rather than having perfect git history

The question is, will the next syncs in both directions actually work, or will there be more duplication? This should be tested.

With josh, the recommended resolution would be a force-push to the "portable-simd" repo to back out the broken-by-rebase syncs. That's what the rust-analyzer folks did when they ran into the same issue. (I see a strong confirmation of my dislike for rebase-on-merge strategies. ;)

Comment on lines -303 to -322
// This used to cause an ICE. It exercises simd_select_bitmask with an array as input.
let bitmask = u8x4::from_array([0b00001101, 0, 0, 0]);
assert_eq!(
mask32x4::from_bitmask_vector(bitmask),
mask32x4::from_array([true, false, true, true]),
);
let bitmask = u8x8::from_array([0b01000101, 0, 0, 0, 0, 0, 0, 0]);
assert_eq!(
mask32x8::from_bitmask_vector(bitmask),
mask32x8::from_array([true, false, true, false, false, false, true, false]),
);
let bitmask =
u8x16::from_array([0b01000101, 0b11110000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
assert_eq!(
mask32x16::from_bitmask_vector(bitmask),
mask32x16::from_array([
true, false, true, false, false, false, true, false, false, false, false, false, true,
true, true, true,
]),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this just tests from_bitmask_vector, so yeah if that API is gone we can remove the test. We still also directly test the intrinsic.

Does this mean simd_select_bitmask on an array is no longer used? What about simd_bitmask with an array?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither intrinsic is used with arrays anymore. The limitation was const generics on the API so I could see it coming back at some point in the future, though

@calebzulawski
Copy link
Member Author

No problem, I just searched zulip for subtrees and it seemed you discussed it a lot, but admittedly in the context of josh. (Once we get this all fixed, switching to josh would be nice...)

The rebasing was mostly a mistake, @workingjubilee and I have manually done subtree syncs and I know I have rebased a couple times. I only just added a sync script to ensure we get this right in the future and that's how I discovered it.

I think this won't be a problem in the future. I know if I do a subtree push now (rust -> portable-simd) the branch it creates is identical to master. Using josh to rewrite bad commit history sounds like a good idea, but I think we would need to at least merge this PR because the portable-simd changes would need to be part of that, right?

@RalfJung
Copy link
Member

Using josh to rewrite bad commit history sounds like a good idea,

I didn't suggest that so I don't know what you are referring to here.

I have no idea how josh will react to these odd duplicated commits if you want to migrate to josh in the future. They will definitely also be duplicated in the portable-simd repo then, and maybe that's it, or maybe it causes more problems -- hard to say without trying.

I know I have rebased a couple times

But then why is this the first time we see duplicated commits? Or is it not the first time?

@calebzulawski
Copy link
Member Author

Sorry I was referring to rewriting portable-simd's history:

With josh, the recommended resolution would be a force-push to the "portable-simd" repo to back out the broken-by-rebase syncs

I think we ultimately want portable-simd's history to look like this repo's, whether that includes these duplicate commits or not. I don't remember seeing this kind of duplicate commit in the past, but if there were any, they're part of this repo already. I'm guessing we just got lucky, because we try to avoid pushing changes to this repo instead of portable-simd.

My main concern is I don't feel confident that I could fix the portable-simd repo (my limited git ability got us in this mess 🙂). I would like to merge this as-is (and accept these duplicate commits), and then make sure we either use the sync script or preferably switch to josh to avoid it in the future. I'm just not ready to merge this without getting some more input.

@RalfJung
Copy link
Member

RalfJung commented Jan 19, 2025

Sorry I was referring to rewriting portable-simd's history:

That would be a manually constructed force-push to before the rebased PR got merged. Josh isn't used for the rewriting, the rewriting is done to make Josh work again. ;)

@calebzulawski
Copy link
Member Author

calebzulawski commented Jan 19, 2025

Here's the complete list of duplicated commits in portable-simd:

git rev-list --format="%ai    %s" origin/master | sed -n '0~2p' | sort | uniq -d
2021-02-14 23:35:24 -0500    Add floating-point classification functions
2021-03-06 02:14:58 -0500    Various bug fixes
2022-04-12 11:01:22 -0400    portable-simd: use simd_arith_offset to avoid ptr-int transmutation
2022-05-20 08:54:10 -0400    Finish bumping stage0
2023-07-16 00:37:30 +0800    remove repetitive words
2023-12-02 10:49:21 -0500    Remove link to core::arch::x86_64
2023-12-12 23:26:45 +0100    Fix target_feature config in portable-simd
2024-01-30 03:40:53 +0000    Disable conversions between portable_simd and stdarch on big-endian ARM
2024-03-23 11:47:11 +0100    rename ptr::from_exposed_addr -> ptr::with_exposed_provenance
2024-03-23 23:00:53 +0100    also rename the SIMD intrinsic
2024-03-25 11:02:02 -0700    Import the 2021 prelude in the core crate
2024-04-03 15:17:00 +0200    rename `expose_addr` to `expose_provenance`

A bunch of them are in this PR, but some of them predate this PR as well and are already in this repo somewhere.
For reference, rust-lang/rust has a ton of duplicate commits:

git rev-list --format="%ai %s" origin/master | sed -n '0~2p' | sort | uniq -d | wc -l
4048

So, do I try to fix everything from around July 2023 for this PR?

@RalfJung
Copy link
Member

RalfJung commented Jan 19, 2025

For reference, rust-lang/rust has a ton of duplicate commits:

Spot-checking two of them, I found clippy and cranelift. So seems like other git subtree based components have the same issue. Not sure if those are caused by rebases as well -- Cc @bjorn3 @rust-lang/clippy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.